Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

disabled custom checkbox and radio background colors #25213

Merged
merged 3 commits into from
Jan 14, 2018

Conversation

gijsbotje
Copy link
Contributor

Fixes #25201 by adding a variable for the background color of this state and adding the styles

Maybe setting the color with rgba is not the right way so i'm open for suggestions on that

… inputs and radios that are disabled

added styles for these cases
@mdo
Copy link
Member

mdo commented Jan 8, 2018

Thanks for this! In my testing, this needs a couple more tweaks that I have in a local branch. Will push an update with that later tonight hopefully.

@mdo mdo self-assigned this Jan 8, 2018
@gijsbotje
Copy link
Contributor Author

awesome, wonder what i missed :)

mdo added a commit that referenced this pull request Jan 10, 2018
This builds on #25213 by ensuring that custom check states don't override the default background-color each time. I've added a new gradient mixin that only includes a conditional gradient instead of a background-color fallback. Without this, the styles added in @gijsbotje's PR would still be overridden.
@mdo mdo merged commit 4a1bc75 into twbs:v4-dev Jan 14, 2018
@mdo mdo added this to Inbox in v4.0 stable via automation Jan 17, 2018
@mdo
Copy link
Member

mdo commented Jan 17, 2018

I don't remember merging this... 🤔

@gijsbotje
Copy link
Contributor Author

wasn't me :D

@mdo
Copy link
Member

mdo commented Jan 17, 2018

@gijsbotje Haha, all good. I have a big explanation for what I had to do in addition to this PR, but it boils down to the following:

  • We should keep all our shared styles to the first .custom-control-input, so we can avoid repeating those selectors for radio and check flavors.
  • As soon as we consolidate them at the top, we hit some issues with other states overriding those styles.
  • The gradient-bg() mixin needs an alternative where it doesn't unnecessarily set a background-color.

There may've been a few more things. My first pass is at https://github.com/twbs/bootstrap/compare/disabled-custom-checks, but I think I have more on my other laptop at home.

@mdo mdo moved this from Inbox to Needs review in v4.0 stable Jan 18, 2018
@mdo
Copy link
Member

mdo commented Jan 18, 2018

Here's what I drafted for that other reply... I need to verify all this still, so trying to do that tonight. Depending on how deep into the rabbit hole I get, might punt on cleaning it up.


As I was reviewing this PR, I saw that you were effectively setting the :disabled:checked styles for radio and checkbox separately. This should be done once on the general .custom-check-input, thereby reduping that selector for the background-color.

Here's the diff for that:

diff --git a/scss/_custom-forms.scss b/scss/_custom-forms.scss
index dd844c07f..281ed2be7 100644
--- a/scss/_custom-forms.scss
+++ b/scss/_custom-forms.scss
@@ -49,6 +49,10 @@
         background-color: $custom-control-indicator-disabled-bg;
       }
     }
+
+    &:checked ~ .custom-control-label::before {
+      background-color: $custom-control-indicator-checked-disabled-bg;
+    }
   }
 }
 
@@ -108,21 +112,16 @@
     }
   }
 
-  .custom-control-input:indeterminate ~ .custom-control-label {
-    &::before {
+  .custom-control-input:indeterminate {
+    ~ .custom-control-label::before {
       @include gradient-bg($custom-checkbox-indicator-indeterminate-bg);
       @include box-shadow($custom-checkbox-indicator-indeterminate-box-shadow);
     }
-    &::after {
+    ~ .custom-control-label::after {
       background-image: $custom-checkbox-indicator-icon-indeterminate;
     }
-  }
 
-  .custom-control-input:disabled {
-    &:checked ~ .custom-control-label::before {
-      background-color: $custom-control-indicator-checked-disabled-bg;
-    }
-    &:indeterminate ~ .custom-control-label::before {
+    &:disabled ~ .custom-control-label::before {
       background-color: $custom-control-indicator-checked-disabled-bg;
     }
   }
@@ -145,12 +144,6 @@
       background-image: $custom-radio-indicator-icon-checked;
     }
   }
-
-  .custom-control-input:disabled {
-    &:checked ~ .custom-control-label::before {
-      background-color: $custom-control-indicator-checked-disabled-bg;
-    }
-  }
 }

Once I made that change, the :disabled:checked was no longer picking up the new background-color you added. Here's the before and after of applying the above diff.

Before After
screen shot 2018-01-12 at 5 46 59 pm screen shot 2018-01-12 at 5 46 45 pm

So, because of that, I needed to make additional changes to find and remove the offending background-color that was overriding your new one. Turns out, it's coming from the gradient-bg() mixin that sets a background-image gradient (if enabled) or a background-color. The latter is unnecessary in these situations. As such, I added another mixin, gradient, to only apply a background-image when gradients are enabled.

@mdo mdo moved this from Needs review to Shipped in v4.0 stable Jan 18, 2018
@gijsbotje
Copy link
Contributor Author

gijsbotje commented Jan 18, 2018

yeah sounds like a better idea to do this form .custom-check-input
I figured, since indeterminate is set at that level, I could add the disabled state there as well
but looks good to me, good luck diving into the rabbit hole 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v4.0 stable
  
Shipped
Development

Successfully merging this pull request may close these issues.

None yet

4 participants